feature: add pubsub (tests and API Spec) #101

haadcode · 2016-12-05T12:13:02Z

This PR will add tests for the upcoming pubsub API.

Note that this PR is dependent on ipfs-inactive/js-ipfs-http-client#465.

victorb · 2016-12-05T12:14:10Z

src/pubsub-message.js

+
+const expect = require('chai').expect
+const isNode = require('detect-node')
+const PubsubMessage = require('../../src/pubsub-message') // eslint-disable-line no-unused-vars


Why include this if we don't need it?

We need it eventually. Leaving it there as a placeholder.

However, these should not be required from '../src' but from ipfs-api. @dignifiedquire how can I do that? interface-ipfs-api tests that refer to components in js-ipfs-api (and the same components will prolly be used in js-ipfs) but are not publicly exposed by the module?

you shouldn't require them, they should be passed in through the setup function

How? How passed what? Can you provide an example?

The tests have a before filter, which calls common.setup, so you can pass more things through there. You could either add the functionality to the factory or as a third parameter.

victorb · 2016-12-05T12:15:25Z

src/pubsub-message.js

+  describe('immutable properties', () => {
+    const message = PubsubMessageUtils.create('A', 'hello', '123', ['hello world'])
+
+    it('from', () => {


If this doesn't throw any exception, won't the test pass?

Should be something like this instead:

let err = null try { message.from = 'not allowed' } catch (e) { err = e } expect(e).to.be.an('error') expect(e.toString()).to.equal(`TypeError: Cannot set property from of #<PubsubMessage> which has only a getter`)

You're right. Will fix this.

Not sure if that's the right fix. You just added a expect in the try part, which would not get run if there is an error and now we're doing different assertions depending on if the code throws an exception or not. The tests should not be able to change the executed code path.

Thanks, all this got me thinking.

I changed it a bit and just pushed a new commit to make it even more implicit that we're testing immutability here: check that the values are the same as before if no error is thrown.

victorb · 2016-12-05T12:21:11Z

src/pubsub.js

+        loop()
+      })
+
+      it('call subscribe 1k times', (done) => {


This is implicitly tested in the subscribe/unsubscribe 1k times, so we can remove this test.

Unsubscribe changes the internal state. This test is meant to make sure that NOT calling unsubscribe before calling subscribe again doesn't change the state and works as expected.

victorb · 2016-12-05T12:21:20Z

src/pubsub.js

+        })
+      })
+
+      it('call publish 1k times', (done) => {


This is implicitly tested in the subscribe/unsubscribe 1k times, so we can remove this test.

Same as above.

daviddias

@haadcode I believe I don't understand correctly the purpose of pubsub.peers, could you clarify/fix the docs please?

daviddias · 2016-12-09T21:42:03Z

src/pubsub.js

+            expect(err.toString()).to.equal(`Error: Not subscribed to '${topic}'`)
+            done()
+          })
+        })


@haadcode why would it fail if I'm not subscribed to a topic? Could you clarify?

My logic here was that the state of the subscriptions is the source of truth: if you're subscribed, you can query it, cancel it, etc. If you're not, you can't.

What would you expect to receive if you're not subscribed to the topic and call .peers()? En empty list?

Say we have a use case like in the tests for the "waitForPeers", we callback only after we received peers. We don't subscribe to the topic but we keep polling pubsub.peers in the hopes that it returns the peer some day. We keep waiting and waiting and it doesn't seem to work, we check connectivity, we start looking elsewhere, because peers just returns an empty array. That's pretty confusing behaviour for the API. In this case, I believe it's better to tell the user with an error that "hey, what' you're maybe trying to do, will not work".

What would you expect to receive if you're not subscribed to the topic and call .peers()? En empty list?

For me peers(topic) tells me I should expect "give me the peers I have in my peerSet that are subscribed to a given topic".

haadcode · 2016-12-10T07:24:58Z

I'm ok merging this when you are.

daviddias

@haadcode found problems running the new enabled tests in js-ipfs-api.

Also, the process.stdout.write won't fly in the browser, right now I'm commenting them out, but it is preferable to find an alternative logging.

daviddias · 2016-12-11T18:44:50Z

src/pubsub.js

@@ -143,7 +154,7 @@ module.exports = (common) => {
          })
        })

-        it.skip('returns no peers within 10 seconds', (done) => {
+        it('returns no peers within 10 seconds', (done) => {


@haadcode this test is failing in js-ipfs-api

daviddias · 2016-12-11T18:45:02Z

src/pubsub.js

@@ -159,7 +170,7 @@ module.exports = (common) => {
          })
        })

-        it.skip('doesn\'t return extra peers', (done) => {
+        it('doesn\'t return extra peers', (done) => {


@haadcode this one too

daviddias · 2016-12-11T18:45:14Z

src/pubsub.js

@@ -182,10 +193,16 @@ module.exports = (common) => {
          })
        })

-        it.skip('returns peers for a topic - one peer', (done) => {
+        it('returns peers for a topic - one peer', (done) => {


@haadcode and this one

daviddias · 2016-12-11T18:45:42Z

src/pubsub.js

@@ -182,10 +193,16 @@ module.exports = (common) => {
          })
        })

-        it.skip('returns peers for a topic - one peer', (done) => {
+        it('returns peers for a topic - one peer', (done) => {
          // Currently go-ipfs returns peers that have not been subscribed to the topic
          // Enable when go-ipfs has been fixed


Why not filter at js-ipfs-api?

daviddias · 2016-12-11T18:49:31Z

I'm ok merging this when you are.

Found some issues when running the new tests you added/enabled in js-ipfs-api. They run well in js-ipfs though.

haadcode · 2016-12-12T09:28:14Z

Found some issues when running the new tests you added/enabled in js-ipfs-api. They run well in js-ipfs though.

@diasdavid this is due to using a (old) version go-ipfs that has bugs for those tests. As far as I know they're now fixed in master but we need to wait for go-ipfs 0.4.5-rc1 release to verify the tests are working.

~~Shall we put this to "blocked" until we have that?~~~

We don't need to put this on hold, but note that when running these tests with js-ipfs-api, one needs to use go-ipfs 0.4.5 binary built from git (master).

daviddias · 2016-12-13T01:18:27Z

src/pubsub.js

+                  outputProgress()
+                  ipfs2.pubsub.publish(topic, expectedString, (err) => {
+                    expect(err).to.not.exist
+                    process.nextTick(() => loop())


@haadcode this is till Node.js specific

dignifiedquire · 2016-12-18T09:58:58Z

API/pubsub/README.md

+- `options` - type: Object, optional, might contain the following properties:
+  - `discover`: type: Boolean - Will use the DHT to find 
+
+`callback` must follow `function (err, subscription) {}` where Subscription is a Node.js Stream in Object mode, emiting a `data` event for each new message on the subscribed topic.`err` is an error if the operation was not successful.


I don't think this is a good interface. We all know the issues of node.js streams and using object streams is even more problematic, as some modules (like hapi.js) don't even work with object streams.

I would propose a simpler api using raw EventEmitter.

const handler = onMessage ipfs.pubsub.subscribe('mytopic', {discover: true}, onMessage) ipfs.pubsub.unsubscribe('mytopic', onMessage)

Which would be implemented using EventEmitter and mostly aliases for .on and removeListener. Where removeListener does a check onto emitter.listenerCount(topic) and if that is 0 it will actually unsubscribe from the underlying thing.

I can implement this, but would like some feedback from @haadcode and @gavinmcdermott before :)

This will require changes in the js-ipfs-api PR too

Absolutely. Let's do it!

Awesome! Please reuse open connections in js-ipfs-api, i.e: If a subscription request is already open, avoid opening a new one, just increase the ref count.

@dignifiedquire - Awesome. Let me know if there's any areas you need help / support in this!

dignifiedquire · 2016-12-18T10:43:55Z

API/pubsub/README.md

+`callback` must follow `function (err) {}` signature, where `err` is an error if the operation was not successful.
+
+If no `callback` is passed, a [promise][] is returned.
+


Missing docs on the value that is resolved

dignifiedquire · 2016-12-18T10:44:06Z

API/pubsub/README.md

+
+- `topic` - type: String
+
+`callback` must follow `function (err) {}` signature, where `err` is an error if the operation was not successful.


Missing docs on the value that is resolved

dignifiedquire · 2016-12-18T10:44:29Z

CHANGELOG.md

+
+### Features
+
+* **object:** add template option to object.new ([2f23617](https://github.com/ipfs/interface-ipfs-core/commit/2f23617))


why is there a changelog in a pr?

It seems that is from a rebase

dignifiedquire · 2016-12-18T10:45:12Z

src/object.js

@@ -41,6 +41,19 @@ module.exports = (common) => {
            done()
          })
        })
+
+        it.skip('template unixfs-dir', (done) => {


this seems entirely unrelated

same as #101 (comment)

dignifiedquire · 2016-12-18T10:46:14Z

src/pubsub.js

+            const topics = ['one', 'two', 'three']
+            series(
+              topics.map((t) => (cb) => ipfs1.pubsub.subscribe(t, cb))
+            , (err, subs) => {


please put the comma at the end of the line

daviddias · 2016-12-18T21:49:21Z

src/pubsub.js

@@ -156,7 +157,8 @@ module.exports = (common) => {
            })
          })

-          it('returns no peers within 10 seconds', (done) => {
+          // I don't understand the purpose of this test
+          it.skip('returns no peers within 10 seconds', (done) => {


@haadcode ^^

daviddias · 2016-12-20T05:43:59Z

src/pubsub.js

+        })
+
+        describe('.peers', () => {
+          it('returns an error when not subscribed to a topic', (done) => {


There was an open question in this interface. I personally don't understand why the question "Tell me who I know that is subscribed to a topic" needs to fail in case I'm not interested to the topic.

@haadcode could you clarify your argument for it to be this way?

@gavinmcdermott @dignifiedquire thoughts?

I would prefer not throwing

As a consumer, I'd be a little surprised to encounter a failure case when asking for a list of the peers subscribed to a topic. Here's the rational behind this for me:

When I'm making this call, I feel like I'm in the mode of asking the following: "Will you please give me a list of all the people I know who are subscribed to this topic?"

To me, the above question feels disconnected from any notion of my subscriptions

I'm not set on this by any means; this is just my mental model.

I'm absolutely fine not throwing an error.

There is a way to look at it as "closed" channels. Kinda like, if you don't participate, you're not allowed to know. But I think this can be achieved with topic CIDs at a later point.

Alright, will change to not throwing behaviour

daviddias · 2016-12-20T05:44:56Z

src/pubsub.js

+
+          it.skip("doesn't return extra peers", (done) => {
+            // Currently go-ipfs returns peers that have not been
+            // subscribed to the topic. Enable when go-ipfs has been fixed


What this means is that js-ipfs-api should filter out those who aren't subscribed

I don't see how js-ipfs-api could do that if go doesn't return tge right values. The api only knows about local subs

I see, fair point :( Is there a feature request for the go-ipfs implementation? @whyrusleeping @Kubuxu ?

I think it was resolved recently, can you give it another try?

It's fixed in master, but not the build we can pull in

daviddias · 2016-12-20T05:45:10Z

src/pubsub.js

+            ], (err) => {
+              expect(err).to.not.exist
+              setTimeout(() => {
+                ipfs1.pubsub.peers(topic, (err, peers) => {


Are these peers, peerInfo objects?

they are peer ids encoded as b58 multihash

It would be awesome to have a way to fetch peer info from the http api, namely known multiaddrs, public key, supported transports, etc //cc @whyrusleeping

daviddias · 2016-12-20T05:45:48Z

src/pubsub.js

+            function sub1 (msg) {
+              inbox1.push(msg.data.toString())
+              // TODO: enable when go-ipfs is unbroken
+              // expect(msg.from).to.be.eql(ipfs2.peerId.id)


Is there a issue that can also be linked here?

yes, it's mentioned in the first case, I just didn't want to duplicated it all over the file

daviddias · 2016-12-20T05:46:07Z

API/pubsub/README.md

+`callback` must follow `function (err) {}` signature, where `err` is an error if the operation was not successful.
+
+If no `callback` is passed, a [promise][] is returned.
+


Please complete

daviddias · 2016-12-20T05:46:14Z

API/pubsub/README.md

+
+- `topic` - type: String
+
+`callback` must follow `function (err) {}` signature, where `err` is an error if the operation was not successful.


Please complete

dignifiedquire · 2016-12-20T14:11:57Z

Ready for next review

gavinmcdermott · 2016-12-20T17:43:03Z

API/pubsub/README.md


 ##### `Go` **WIP**

 ##### `JavaScript` - ipfs.pubsub.publish(topic, data, callback)

- `topic` - type: String
- `data` - type: Buffer


Did you intentionally remove the data argument?

daviddias · 2016-12-20T19:26:52Z

API/pubsub/README.md

+##### `JavaScript` - ipfs.pubsub.publish(topic, data, callback)
+
+- `topic: string`
+- `callback: (Error) => ()` - Calls back with an error or nothing if the publish was successfull.


It is missing the description of data

daviddias · 2016-12-20T19:33:41Z

src/pubsub.js

+        describe('.publish', () => {
+          it('message from string', (done) => {
+            ipfs1.pubsub.publish(topic, 'hello friend', done)
+          })


I'm worried with accepting plain strings as argument. The issue that we had last week Monday was because the initial pubsub code @haadcode wrote would always convert any value to a Buffer on the publish implementation, but then, because the tests were written like expect(dataReceived).to.equal('some string'), the implementation had a data.toString() internally as a quick patch, and when I changed that to make it an operation in the outside (like we have here now, see below, preserving the buffer type), it made orbit stop working because there was an assumption that 'data' was a string, since orbit publishes strings (or was publishing).

Strings and Buffers are one of banes of JavaScript, it would be better to force it to be just Buffer and no other type (creating a test here to make sure it fails with anything non buffer), to avoid future problems.

We could also check strictly for string or Buffer, but nothing else, which makes it a bit nicer on the user, but avoids issues like passing an array by accident.

Note that the case I present was exactly because of a user expecting if it published a 'string', it would come out a 'string'.

But yeah, at least, restrict everything else.

which makes it a bit nicer on the user

The other option would be to have a big bold note "Any string will be converted to a Buffer"

needs to be resolved before the merge

I agree @diasdavid that this is something we should decide now as it has bitten us already in a bad way. What do we do in other function that accept similar data as a param, like object.put? It might be a good idea to follow similar convention. If it's string+buffer, then what David proposes makes sense (always convert to buffer and make it as explicit to the user as possible (docs + erroring on anything but string or buffer). If it's only a buffer, it would also make sense to me (input buffer, receive buffer). This is how I tried to approach it in js-ipfs-api: everything we receive is a string, so inputs should be strings (from user's perspective). Obviously we would need to change js-ipfs-api to convert all strings to buffers, and probably in a lot of use cases the user then converts the buffer back to a string, but I suppose it's a cost we could live with (and on a positive side, js-ipfs would be tiny bit faster because it only needs to use buffers).

So... I think I like the idea of accepting buffers-only the most, but it wouldn't be too bad if we accepted strings too.

alright, going to change things to buffer only then

daviddias

Minor things, the String vs Buffer needs a resolution, otherwise LGTM 👍

daviddias · 2016-12-21T06:19:37Z

API/pubsub/README.md

+
+- `topic: string`
+- `options: Object` - (Optional), might contain the following properties:
+  - `discover`: type: Boolean - Will use the DHT to find


"to find.." - sentence needs to be finished

I think this is a great sentence :P

daviddias · 2016-12-21T06:20:24Z

API/pubsub/README.md

+##### `JavaScript` - ipfs.pubsub.publish(topic, data, callback)
+
+- `topic: string`
+- `callback: (Error) => ()` - Calls back with an error or nothing if the publish was successfull.


daviddias · 2016-12-21T06:21:21Z

API/pubsub/README.md

+- `topic: string`
+- `callback: (Error) => ()` - Calls back with an error or nothing if the publish was successfull.
+
+- Returns: `Promise` if no `callback` was passed otherwise `undefined`.


We have used always: If no callback is passed, a [promise][] is returned., I'm fine changing the sentence, but I do like consistency :)

changed back

daviddias · 2016-12-21T06:22:10Z

src/object.js

@@ -42,7 +42,7 @@ module.exports = (common) => {
          })
        })

-        it('template unixfs-dir', (done) => {
+        it.skip('template unixfs-dir', (done) => {


Why do we need to skip this one?

I don't even know why this is here in the first place

daviddias · 2016-12-21T06:22:51Z

src/pubsub.js

+      done()
+    }
+  }
+}


Can't we use the chai checkmark thing?

I honestly found this to be simpler to use and understand. But not opposed to switching.

Sounds good :)

daviddias · 2016-12-21T06:23:26Z

src/pubsub.js

+        describe('.publish', () => {
+          it('message from string', (done) => {
+            ipfs1.pubsub.publish(topic, 'hello friend', done)
+          })


needs to be resolved before the merge

daviddias · 2016-12-21T06:26:49Z

src/pubsub.js

+            const handler2 = (msg) => {
+              expect(msg.data.toString()).to.be.eql('hello')
+              check()
+            }


We've been kind of declaring functions always with the word function, it has some benefits as making it easy to grep. We kind of have followed (naturally) the pattern of always using function throughout the code and I would like to keep it consistent, unless there is a strong case not to.

You have used function I try to use const name = () especially inside other functions and to avoid using functions before they are declared, as it is very unfortunate that function declarations are hoisted to the top level, allowing for code to use functions before they have been declared.

daviddias · 2016-12-21T06:27:30Z

src/pubsub.js

+              return done(err)
+            }
+            // give some time to let everything connect
+            setTimeout(done, 300)


Does this need to be here? Will it fail it it doesn't?

…s-api

dignifiedquire · 2016-12-21T15:45:52Z

🎉 100 comments

haadcode added the in progress label Dec 5, 2016

victorb reviewed Dec 5, 2016

View reviewed changes

haadcode force-pushed the tests/pubsub branch from e00d8e7 to 638dc65 Compare December 5, 2016 12:14

victorb reviewed Dec 5, 2016

View reviewed changes

haadcode force-pushed the tests/pubsub branch 2 times, most recently from 0a221fc to c9b9751 Compare December 5, 2016 13:53

This was referenced Dec 5, 2016

track: floodsub ipfs/js-ipfs#580

Closed

WIP: feat/pubsub \o/ ipfs/js-ipfs#610

Merged

haadcode self-assigned this Dec 5, 2016

haadcode added the needs review label Dec 5, 2016

daviddias removed the needs review label Dec 6, 2016

daviddias changed the title ~~feature: Add Pubsub Tests~~ feature: add pubsub (tests and API Spec) Dec 6, 2016

daviddias mentioned this pull request Dec 6, 2016

FloodSub API discussion ipfs/js-ipfs#530

Closed

haadcode force-pushed the tests/pubsub branch from 5eadaff to 501c304 Compare December 9, 2016 11:02

daviddias self-assigned this Dec 9, 2016

daviddias suggested changes Dec 9, 2016

View reviewed changes

daviddias suggested changes Dec 11, 2016

View reviewed changes

daviddias mentioned this pull request Dec 11, 2016

Feature: PubSub 🌟 ipfs/js-ipfs#644

Merged

haadcode added ready and removed in progress labels Dec 12, 2016

daviddias suggested changes Dec 13, 2016

View reviewed changes

dignifiedquire reviewed Dec 18, 2016

View reviewed changes

daviddias reviewed Dec 18, 2016

View reviewed changes

dignifiedquire force-pushed the tests/pubsub branch from dab167d to 3adf1ae Compare December 19, 2016 10:06

daviddias added in progress and removed ready labels Dec 19, 2016

daviddias assigned dignifiedquire Dec 19, 2016

daviddias suggested changes Dec 20, 2016

View reviewed changes

dignifiedquire added the needs review label Dec 20, 2016

gavinmcdermott reviewed Dec 20, 2016

View reviewed changes

daviddias reviewed Dec 20, 2016

View reviewed changes

daviddias removed the needs review label Dec 21, 2016

daviddias suggested changes Dec 21, 2016

View reviewed changes

daviddias and others added 8 commits December 21, 2016 15:16

chore: update contributors

562a0bb

chore: release version v0.22.1

e9506ff

chore: update deps

56cd45e

feat: add first pass of pubsub tests (running in js-ipfs-api)

74003a7

moved pubsub-message tests to js-ipfs-api as it is specific to js-ipf…

eb65994

…s-api

docs: add PubSub interface spec, refactor tests

a63988b

fix: fix a bunch of issues (i.e: identify race condition)

d004577

feat: new event based API

1a82890

daviddias force-pushed the tests/pubsub branch from 20392b8 to 1a82890 Compare December 21, 2016 15:20

daviddias approved these changes Dec 21, 2016

View reviewed changes

Merge branch 'master' into tests/pubsub

142c417

daviddias merged commit 99a6bc4 into master Dec 21, 2016

daviddias deleted the tests/pubsub branch December 21, 2016 15:21

daviddias removed the in progress label Dec 21, 2016

		`callback` must follow `function (err) {}` signature, where `err` is an error if the operation was not successful.

		If no `callback` is passed, a [promise][] is returned.


		- `topic` - type: String

		`callback` must follow `function (err) {}` signature, where `err` is an error if the operation was not successful.


		### Features

		* object: add template option to object.new ([2f23617](https://github.com/ipfs/interface-ipfs-core/commit/2f23617))

feature: add pubsub (tests and API Spec) #101

feature: add pubsub (tests and API Spec) #101

Conversation

haadcode commented Dec 5, 2016 • edited by dignifiedquire Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorb Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

haadcode commented Dec 10, 2016

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Dec 11, 2016

haadcode commented Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinmcdermott Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode commented Dec 5, 2016 •

edited by dignifiedquire

Loading

dignifiedquire Dec 5, 2016 •

edited

Loading

victorb Dec 5, 2016 •

edited

Loading

haadcode Dec 5, 2016 •

edited

Loading

daviddias Dec 10, 2016 •

edited

Loading

haadcode commented Dec 12, 2016 •

edited

Loading

dignifiedquire Dec 18, 2016 •

edited

Loading

gavinmcdermott Dec 20, 2016 •

edited

Loading

dignifiedquire Dec 20, 2016 •

edited

Loading